-
-
Notifications
You must be signed in to change notification settings - Fork 833
Add in a "verify" slash command to confirm signing keys #912
Conversation
Can one of the admins verify this patch? |
Ack, messed up the error dialog in my hurry, one moment... |
9f3b2b5
to
9336642
Compare
src/SlashCommands.js
Outdated
// Verify a user, device, and pubkey tuple | ||
verify: new Command("verify", "<userId> <deviceId> <deviceSigningKey>", function(room_id, args) { | ||
if (args) { | ||
var matches = args.match(/^(\S+) +(\S+) +(\S+)?$/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure whether it'd be more sensible to use more specific character groups, since all 3 have specific grammars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I guess I was trying to avoid having a complex regex considering all the information is checked against APIs which accept arbitrary strings. Is there any reason to make this more specific? Any recommendations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to go for that path then maybe simpler using String.prototype.split
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW the other code in this file follows this Regex approach to parse out inputs. Probably should be properly validated and sanitized, but that's kinda unrelated to this change.
src/SlashCommands.js
Outdated
</div> | ||
), | ||
button: "Accept this validated key", | ||
onFinished: confirm=>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be onFinished: (confirm) => {
to make ESLint happy :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix! Side note: you should also fix this in onVerifyClick
in src/components/views/elements/DeviceVerifyButtons.js
src/SlashCommands.js
Outdated
} | ||
} | ||
} | ||
return reject(this.getUsage()); | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a trailing comma plox
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
src/SlashCommands.js
Outdated
} else { | ||
return reject("WARNING: KEY VALIDATION FAILED! The signing key for " + userId + " and device " | ||
+ deviceId + " is \"" + device.getFingerprint() + "\" which does not match the provided key \"" | ||
+ fingerprint + "\" This could mean your communications are being intercepted!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe toy with template literals to make the string interpolation a little neater
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll see if I can clean this up.
Test build deployed to: https://riot.ovh/builds/oob-verify-device/ |
Seeing EDIT: fixed |
src/SlashCommands.js
Outdated
// Verify a user, device, and pubkey tuple | ||
verify: new Command("verify", "<userId> <deviceId> <deviceSigningKey>", function(room_id, args) { | ||
if (args) { | ||
var matches = args.match(/^(\S+) +(\S+) +(\S+)?$/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the last capture group (for deviceSigningKey
) be mandatory? Otherwise, line 299 can get an undefined. And then line 301 would be comparing the device fingerprint against nothing. The call will fail, which is fine. But it seems odd to have a verify method with the actual verifiy datum optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh man, how did I miss that?! Yes, sorry, that is a terribly silly bug. Will fix.
src/SlashCommands.js
Outdated
|
||
var QuestionDialog = sdk.getComponent("dialogs.QuestionDialog"); | ||
return success(Modal.createDialog(QuestionDialog, { | ||
title: "Approve Validated device", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent casing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, this is a copy-paste induced casing problem. Will drop the 'Q' down to lowercase.
Derp. I mean I will drop the 'V' in "Validated" lowercase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I meant the title. But yeah that too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t3chguy requested that the Q in QuestionDialog remains capital to be consistent with other examples in this file (see ErrorDialog line 62, for example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -65,7 +65,8 @@ export default React.createClass({ | |||
<ul> | |||
<li><label>Device name:</label> <span>{ this.state.device.getDisplayName() }</span></li> | |||
<li><label>Device ID:</label> <span><code>{ this.state.device.deviceId}</code></span></li> | |||
<li><label>Device key:</label> <span><code><b>{ this.state.device.getFingerprint() }</b></code></span></li> | |||
<li><label>Device fingerprint:</label> <span><code><b>{ this.state.device.getFingerprint() }</b></code></span></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against changing this, but we need to keep the Settings screen consistent, so that people know what they are supposed to be comparing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I find it confusing that we have different names for it different places. I'd prefer (in code) if we referred to them as "signing key" and something else (maybe "stream key"?). "Fingerprint" seems to imply that it's a partial key, or something one could use to quickly verify matches, not that it is our primary source of trust for this user.
Would you be opposed to me making a different PR with "fingerprint" replaced by "signing key" or something similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pulled this because the refactoring of the verify device dialog made it a difficult merge anyway. I would like to have a longer discussion at some point about the "streaming key" at some point! As I pointed out in our chat a few weeks ago, I'm not terribly clear on why each user+device pairing doesn't have a unique streaming key (currently both keys are basically permanent).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to keep it separate. Sorry for causing conflicts.
I agree that there is a bit of a naming confusion going on at the moment. "Signing key" and "encryption key" might be better terms, though tbh in the UI I would prefer the user to be able to forget there is more than one key.
@@ -65,7 +65,8 @@ export default React.createClass({ | |||
<ul> | |||
<li><label>Device name:</label> <span>{ this.state.device.getDisplayName() }</span></li> | |||
<li><label>Device ID:</label> <span><code>{ this.state.device.deviceId}</code></span></li> | |||
<li><label>Device key:</label> <span><code><b>{ this.state.device.getFingerprint() }</b></code></span></li> | |||
<li><label>Device fingerprint:</label> <span><code><b>{ this.state.device.getFingerprint() }</b></code></span></li> | |||
<li><label>Device key:</label> <span><code><b>{ this.state.device.getIdentityKey() }</b></code></span></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and I'd prefer not to show this key at all - it's not useful for the verification process and I worry it will confuse people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it may not be useful at the moment, I wish there was somewhere that it was exposed to the user so they could see the other "public key" they trust. If someone's signing key was used to sign "0" as the streaming key, I would be concerned, and it's just odd that you can't find it in the UI (as far as I can tell).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also removed in the merge.
src/SlashCommands.js
Outdated
<li><label>Device name:</label> <span>{ device.getDisplayName() }</span></li> | ||
<li><label>Device ID:</label> <span><code>{ device.deviceId}</code></span></li> | ||
<li><label>Device fingerprint:</label> <span><code><b>{ device.getFingerprint() }</b></code></span></li> | ||
<li><label>Device key:</label> <span><code><b>{ device.getIdentityKey() }</b></code></span></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, no need for this.
src/SlashCommands.js
Outdated
} else { | ||
return reject(`WARNING: KEY VERIFICATION FAILED! The signing key for ${userId} and device | ||
${deviceId} is "${device.getFingerprint()}" which does not match the provided key | ||
"${fingerprint}" This could mean your communications are being intercepted!`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, yes, good catch.
src/SlashCommands.js
Outdated
<div> | ||
<p> | ||
The signing key in your slash command matches the signing key you | ||
received for this user and device! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The !
marks make this sound a bit overexcitable ;). Can you just use .
please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok!
... I mean "Ok."
src/SlashCommands.js
Outdated
</ul> | ||
</div> | ||
<p> | ||
If you would like to accept this device as verified, press accept! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the text on the button is "Accept this verified key", I think this text is redundant.
src/SlashCommands.js
Outdated
description: ( | ||
<div> | ||
<p> | ||
The signing key in your slash command matches the signing key you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit undecided about whether we need a confirmation dialog here. I think that by the time you're pasting /verify commands in, it's up to you to have made sure that they've come from a trustworthy source - I'm not sure what purpose an extra confirmation serves here.
One obvious danger is that inexperienced users might be persuaded to c&p a /verify command from a malicious user. A better solution to that might be to pop up a dialog explaining what's going on the first time they run a /verify command (and store a flag so they don't have to do it each time).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought was along the lines of future applications: in this flow it's unlikely to matter, but in the future if this was invoked through some other mechanism, I would expect you just want a last-minute "hey, you intended to do this, didn't you?" I was basically assuming "in the future you might invoke this dialog through a QR code, and it shows you what's about to happen before it happens."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said in #riot-dev: if you haven't invoked this functionality via a slash-command, you may want a confirmation dialog. But in that case, the confirmation functionality doesn't belong in SlashCommands.js.
Allows users to send a text string via an alternative channel (like email or SMS) which Riot can leverage to confirm that the signing keys match. Effectively removes the tedium of checking keys until a better mechanism is completed. Signed-off-by: Kit Sczudlo <[email protected]>
Replace all instaces of "Validated" with "Verified", and error out when the user's device is already verified. Signed-off-by: Kit Sczudlo <[email protected]>
Signed-off-by: Kit Sczudlo <[email protected]>
Also remove a few unnecessary escape characters in front or double quotes Signed-off-by: Kit Sczudlo <[email protected]>
Signed-off-by: Kit Sczudlo <[email protected]>
Signed-off-by: Kit Sczudlo <[email protected]>
Signed-off-by: Kit Sczudlo <[email protected]>
Signed-off-by: Kit Sczudlo <[email protected]>
looks good, thanks! |
Allows users to send a text string via an alternative channel (like email or SMS) which Riot can leverage to confirm that the signing keys match. Effectively removes the tedium of checking keys until a better mechanism is completed. Signed-off-by: Kit Sczudlo <[email protected]>
merged as 26c8540 |
Allows users to send a text string via an alternative channel (like email
or SMS) which Riot can leverage to confirm that the signing keys match.
Effectively removes the tedium of checking keys until a better mechanism
is completed.
Note that this is dependent on matrix-org/matrix-js-sdk#439